Did this icon file come from a particular place in the Figma design, or somewhere else?

Please mention the source in the commit message (with e.g. a Figma link). That helps us track where we've gotten our icons from.

Hmm, this if-mounted check at the top isn't something we have in any of the comparable methods in this file. I think we can leave it out.

(If there is a reason we need it, then that reason will probably apply equally to all the other buttons.)

Was there a discussion about this in a previous round of review? It's a bit difficult for me to find out from the GitHub page, because of some of GitHub's UI choices. You can mitigate that by avoiding marking comments as "resolved", similar to what Chris said the other day elsewhere: #1317 (review)

No, this was not discussed in the previous round. Also, I will not mark the comments as resolved from now on.

nit: can simplify with a handy test helper:

This test is implicitly relying on the fact that eg.streamMessage returns a message which is unread (doesn't have the read flag).

In general, test cases should avoid depending on the specific default choices made by the functions in example_data — each test should be self-contained in that the reader should be able to understand the key logic of how it works just by reading the test itself (and any local helper functions specific to the test), without going and looking through the details of example_data.dart.

Instead, both this test and the others that care about whether the message is read or unread should explicitly pass flags to eg.streamMessage so that they control that fact about the message.

It's a bit subtle, but the way this test currently works is depending on a bug. 🙂 Namely #649.

The only difference between the setup of this test and the one above it, which gets the opposite result, is that the other test has the message arrive first as an event and then via a fetch, while this one has it arrive only via a fetch. In both cases the message is unread. If we fetch some messages and one of them is described as unread (it lacks the read flag), then logically there is an unread message in that topic, and the fact that we don't put it into our data structures as such is a bug (namely #649).

So let's instead have this test set things up in a way such that it'll automatically do the right thing even after we fix that bug.

(This also makes an example of why it's valuable to have the test be explicit about the key facts it's relying on in its test data — if this eg.streamMessage line were explicitly saying the message is unread, instead of implicitly doing so, then it'd naturally raise the question of how that fits with the test description saying this is about not having any unreads.)

nit: can simplify a bit with a test helper:

This is a type mismatch: apiEncode returns a list of ApiNarrowElement, but this last item is just a Map<String, String>. It works because it's just getting passed to jsonEncode which is pretty dynamic… but let's clean it up with uniform types. You can do that by putting an appropriate ApiNarrowElement here instead.

This checks it's doing something to the message flags in this narrow — but not that it's marking as read. In particular this test would equally well pass if the implementation were changed to mark the messages as unread instead of read.

Let's add the checks that ensure what it's doing is marking as read, rather than marking as unread or setting or unsetting some other flag.

This last sentence isn't accurate — this function doesn't do anything to unread counts in the UI.

(It wouldn't, because it's the model's job to drive the data that appears in the UI. But this doesn't do anything to unread counts in the model, either. If you're thinking of the PerAccountStoreWidget.of(context).unreads.handleAllMessagesReadSuccess() call at the end, see that method's doc.)

"handle feedback" sounds like something that receives feedback and does something about it. These do the opposite, showing feedback to the user.